Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update location-retrieval with add maxSurface management #262

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

bigludo7
Copy link
Collaborator

@bigludo7 bigludo7 commented Oct 1, 2024

What type of PR is this?

Add one of the following kinds:

  • enhancement/feature

What this PR does / why we need it:

add maxSurface management

Which issue(s) this PR fixes:

Fixes #255

Special notes for reviewers:

Changelog input

 release-note
- Add management of maxSurface in request.

Additional documentation

This section can be blank.

docs

Add `maxSurface` management
Copy link

github-actions bot commented Oct 1, 2024

🦙 MegaLinter status: ✅ SUCCESS

Descriptor Linter Files Fixed Errors Elapsed time
✅ ACTION actionlint 2 0 0.03s
✅ OPENAPI spectral 3 0 4.89s
✅ REPOSITORY git_diff yes no 0.01s
✅ REPOSITORY secretlint yes no 0.79s
✅ YAML yamllint 3 0 0.77s

See detailed report in MegaLinter reports

MegaLinter is graciously provided by OX Security

@@ -525,6 +545,13 @@ components:
status: 422
code: LOCATION_RETRIEVAL.UNABLE_TO_FULFILL_MAX_AGE
message: "Unable to provide expected freshness for location"
LOCATION_RETRIEVAL_422_UNABLE_TO_FULFILL_MAX_SURFACE:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the calculated area violates both age and surface conditions, is the implementer free to choose which 422 code to return ?
As an implementation note, we could recommend to check the age condition first because area comparison can require more computation than age comparison.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I've added a note: if both maxAge and maxSurface requirements fail, the system can either send back one or the other error code.

I agree on your recommendation but not sure if we have to add in the yaml as it is more for the system than the client.

Fixing linting + adding precision as suggested by @alpaycetin74: 
`Note: if both `maxAge` and `maxSurface` requirements fail, the system can either send back one or the other error code.  `
jlurien
jlurien previously approved these changes Oct 7, 2024
Copy link
Collaborator

@jlurien jlurien left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with minor correction and comment

code/API_definitions/location-retrieval.yaml Outdated Show resolved Hide resolved

maxSurface:
type: integer
minimum: 1
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what would be realistically the minimum that a "best" implemention may provide, but likely above 100 m2. We may set 1 here and let implementations provide the minimum supported separately.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we allow a min radius of 1 probably we cannot have a min of 100m2. Probably good to add an note indicating that implementations will provide minimum.

Add a note for maxSurface minimum allowed.
* **Max Surface**: Maximum surface in square meters which is accepted by the client for the location retrieval.

* absence of `maxSurface` means that "any surface size" is acceptable for the client.
* API implementation could specify in the documentation the `maxSurface` minimum acceptable (for exemple minimun of 10000 square meters allowed).
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to propose a minor rewording here, and fixing some typos:
API implementation could specify the minimum acceptable maxSurface in the documentation (for example a minimum of 10000 square meters are allowed).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

How to understand the response parameter area in the device retrieval API
3 participants